fix #12583: Inverted file existence check in DefaultTransport.put() - #12612
Conversation
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
The core bug fix is correct — the inverted Files.isRegularFile(source) condition on line 101 of DefaultTransport.put() is properly fixed by adding the missing ! negation. The existing code was throwing IllegalArgumentException when the file IS a regular file, which is backwards per the Transport.put() API contract. The bug was introduced in the original Transport API commit (f70b001, 2022-12-01).
A few notes:
-
Merge conflict / rebase needed — The PR is in a conflicting state because the branch was forked before commit 8134aa2 (which moved
DefaultTransport.javato theorg.apache.maven.implpackage). The diff currently shows the entire file as new rather than the single-character fix. A rebase on currentmasterwould produce a clean, minimal diff. -
Unused
@TempDirparameter —testPutBytesSucceedsdeclares@TempDir Path tempDirbut never uses it. It can be removed. -
Test assertions could be stronger — Both
testPutWithExistingFileSucceedsandtestPutBytesSucceedsuse onlyassertDoesNotThrowwithout verifying the mock transporter'sput()method was actually called. Addingverify(transporter).put(any(PutTask.class))would confirm the delegation chain works as expected.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of gnodet
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes a regression in DefaultTransport.put() where the file existence check was inverted, causing valid files to be rejected and missing files to be accepted.
Changes:
- Corrected the
Files.isRegularFile(source)guard by adding the missing negation. - Added JUnit tests covering missing/valid source files and
putBytes()behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultTransport.java | Fixes the inverted isRegularFile condition to properly validate the source file. |
| impl/maven-impl/src/test/java/org/apache/maven/impl/DefaultTransportTest.java | Adds tests for put() source validation and basic putBytes() success behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| void testPutWithNonExistentFileThrows() { | ||
| Transporter transporter = mock(Transporter.class); | ||
| DefaultTransport transport = new DefaultTransport(URI.create("http://example.com/test/"), transporter); | ||
| Path nonExistentFile = Path.of("/nonexistent/file.txt"); | ||
| assertThrows(IllegalArgumentException.class, () -> transport.put(nonExistentFile, URI.create("dest.txt"))); | ||
| } |
| @Test | ||
| void testPutBytesSucceeds(@TempDir Path tempDir) { | ||
| Transporter transporter = mock(Transporter.class); | ||
| DefaultTransport transport = new DefaultTransport(URI.create("http://example.com/test/"), transporter); | ||
| URI dest = URI.create("dest.txt"); | ||
| assertDoesNotThrow(() -> transport.putBytes("test content".getBytes(), dest)); | ||
| } |
| } | ||
|
|
||
| @Test | ||
| void testPutBytesSucceeds(@TempDir Path tempDir) { |
- Use @tempdir for non-existent file test instead of hardcoded path - Remove unused @tempdir parameter from testPutBytesSucceeds - Replace assertDoesNotThrow with verify(transporter).put() to confirm delegation chain actually invokes the transporter Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
All three findings from the previous review have been addressed:
- Merge conflict / rebase — resolved. The diff now cleanly shows the single-line fix.
- Unused
@TempDirparameter — removed fromtestPutBytesSucceeds. - Missing
verify()calls — bothtestPutWithExistingFileSucceedsandtestPutBytesSucceedsnow verify thattransporter.put()was actually called.
The fix is correct — the missing ! negation in the file-existence check is properly added, and the tests cover both positive and negative paths plus the putBytes delegation chain.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of gnodet
|
@gnodet Please assign appropriate label to PR according to the type of change. |
The condition Files.isRegularFile(source) in DefaultTransport.put() was inverted — it threw when the file existed and silently accepted non-existent files, breaking putBytes() and putString() transitively. Added the missing negation and a new DefaultTransportTest covering non-existent file rejection, valid file acceptance, and the putBytes() delegation chain. Backport of #12612
The condition
Files.isRegularFile(source)inDefaultTransport.put()was inverted — it should have been!Files.isRegularFile(source). This causedput()to reject valid existing files and silently accept non-existent ones, which brokeputBytes()andputString()transitively.Fix: Added the missing
!negation operator.Tests: Added
DefaultTransportTestwith tests for non-existent file rejection, valid file acceptance, and theputBytes()->put()delegation chain.Closes #12583